test_runner: remove failure attribute in junit_report testcase element#59685
Open
devholic22 wants to merge 2 commits intonodejs:mainfrom
Open
test_runner: remove failure attribute in junit_report testcase element#59685devholic22 wants to merge 2 commits intonodejs:mainfrom
devholic22 wants to merge 2 commits intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
needs-ci
jasnell
reviewed
Aug 30, 2025
| children: [inspectWithNoCustomRetry(error, inspectOptions)], | ||
| }); | ||
| currentTest.failures = 1; | ||
| currentTest.attrs.failure = error?.message ?? ''; |
Member
There was a problem hiding this comment.
While the change the good, I wonder if we have to treat this as potentially breaking? @nodejs/test_runner @nodejs/tsc any thoughts? I'm fine landing as a semver-patch but want to be sure.
jasnell
approved these changes
Aug 30, 2025
MoLow
approved these changes
Aug 31, 2025
Member
MoLow
left a comment
MoLow
left a comment
There was a problem hiding this comment.
LGTM. also not sure rgarding semver-major vs patch
atlowChemi
approved these changes
Aug 31, 2025
lpinca
approved these changes
Sep 1, 2025
github-actions
bot
removed
the
request-ci
Collaborator
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59685 +/- ##
==========================================
- Coverage 89.91% 89.87% -0.04%
==========================================
Files 667 667
Lines 196600 196855 +255
Branches 38601 38646 +45
==========================================
+ Hits 176780 176933 +153
- Misses 12269 12359 +90
- Partials 7551 7563 +12
🚀 New features to boost your workflow:
|
benjamingr
approved these changes
Sep 1, 2025
|
Hello @atlowChemi #59593 is auto-closed because #60274 has been merged, thank you. Probably this PR also needs a ping, or should we reopen the issue #59593? |
Member
|
The commit message here needs an amend |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #59593
This PR aligns the Node.js test runner’s JUnit XML output with the official JUnit specification by removing the non-standard failure attribute from elements and replacing it with a proper nested tag.
Changes
• Remove non-compliant failure attribute from elements
• Update snapshot tests accordingly
Background
The JUnit XML specification mandates that failure details must be represented via a nested (child)
<failure>element inside a<testcase>, rather than as an attribute.However, the Node.js test runner previously:
• Incorrectly embedded the failure message as a failure="..." attribute on , which is not spec-compliant
• This caused compatibility issues with tools like GitLab, Jenkins, or any CI/CD pipeline that parses JUnit reports strictly
Before:
After:
Testing